Conversation
…ked, and computation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive board metrics feature, including a new MetricsController, BoardMetricsService, and a corresponding frontend dashboard in Vue. The implementation covers key Agile metrics such as throughput, cycle time, work-in-progress (WIP), and blocked card tracking. While the architectural foundation is solid, several issues were identified regarding the accuracy of the metrics. Specifically, the service currently relies on the UpdatedAt timestamp for throughput, cycle time, and blocked duration calculations, which is unreliable as it changes with any card modification. The feedback highlights the need to utilize the already-queried audit logs to determine precise transition timestamps. Additionally, improvements are suggested for handling gaps in trend data, avoiding magic numbers for log limits, and optimizing frontend template logic.
| var completedCards = cards | ||
| .Where(c => c.ColumnId == doneColumn.Id | ||
| && c.UpdatedAt >= from | ||
| && c.UpdatedAt <= to) |
There was a problem hiding this comment.
Using c.UpdatedAt to determine completion time for throughput is unreliable. The UpdatedAt timestamp changes whenever any property of the card is modified (e.g., editing the description or adding a comment). To accurately calculate throughput, you should use the auditLogs to find the specific timestamp when the card transitioned into the doneColumn.
| var entries = doneCards | ||
| .Select(c => | ||
| { | ||
| var cycleTime = (c.UpdatedAt - c.CreatedAt).TotalDays; |
There was a problem hiding this comment.
Using c.UpdatedAt as the completion timestamp for cycle time is incorrect. If a user updates a card's details after it has already reached the 'Done' column, the cycle time will erroneously increase. This calculation should use the transition timestamp from the audit logs.
| var auditLogs = (await _unitOfWork.AuditLogs.QueryAsync( | ||
| query.From, | ||
| query.To, | ||
| boardId: query.BoardId, | ||
| limit: 10000, | ||
| cancellationToken: cancellationToken)).ToList(); |
There was a problem hiding this comment.
The auditLogs variable is queried and populated here but is never actually used in the subsequent logic for computing throughput or cycle time. Based on the PR description, these logs are intended to be the source of truth for card transitions. Currently, the service is using UpdatedAt, which is unreliable.
| query.From, | ||
| query.To, | ||
| boardId: query.BoardId, | ||
| limit: 10000, |
There was a problem hiding this comment.
The hardcoded limit of 10000 audit logs is a magic number that may be insufficient for boards with high activity over long date ranges, or potentially inefficient for smaller boards. Consider making this limit configurable or defining it as a named constant.
| var grouped = completedCards | ||
| .GroupBy(c => c.UpdatedAt.Date) | ||
| .Select(g => new ThroughputDataPoint( | ||
| new DateTimeOffset(g.Key, TimeSpan.Zero), | ||
| g.Count())) | ||
| .OrderBy(dp => dp.Date) | ||
| .ToList(); |
There was a problem hiding this comment.
The throughput grouping only returns data points for dates where cards were completed. This results in 'gaps' in the trend data for days with zero completions, which can lead to misleading visualizations in the frontend chart. It is better to iterate through the date range and fill in missing dates with a CompletedCount of 0.
| .Select(c => | ||
| { | ||
| // Estimate blocked duration from UpdatedAt (when blocked was set) to now | ||
| var blockedDuration = (DateTimeOffset.UtcNow - c.UpdatedAt).TotalDays; |
There was a problem hiding this comment.
Estimating blocked duration using UpdatedAt is inaccurate. If a blocked card is updated (e.g., a comment is added) while it remains blocked, the UpdatedAt timestamp will refresh, causing the calculated duration to be significantly underestimated. The system should ideally track the specific BlockedAt timestamp.
| if (isDemoMode) { | ||
| loading.value = true | ||
| error.value = null | ||
| metrics.value = null | ||
| loading.value = false | ||
| return | ||
| } |
There was a problem hiding this comment.
The demo mode implementation currently clears the metrics state and returns. This results in an empty dashboard for demo users. To provide a meaningful experience as suggested in the PR summary, this should populate the metrics state with realistic mock data.
| <div class="td-metrics__card"> | ||
| <span class="td-metrics__card-label">Total Throughput</span> | ||
| <span class="td-metrics__card-value"> | ||
| {{ metrics.throughput.reduce((sum, d) => sum + d.completedCount, 0) }} |
There was a problem hiding this comment.
Performing a reduce operation directly inside the template can be inefficient as it may be re-evaluated more often than necessary. Consider moving this logic into a computed property (e.g., totalThroughput) for better performance and cleaner template code.
Adversarial Self-ReviewIssues found and fixed
Known limitations documented
What looks good
|
There was a problem hiding this comment.
Pull request overview
Adds a new “Board Metrics” feature to Taskdeck, spanning a backend metrics computation endpoint and a frontend dashboard view for throughput, cycle time, WIP, and blocked-card reporting.
Changes:
- Backend: introduce board-metrics DTOs, application service, and
GET /api/metrics/boards/{boardId}endpoint with DI wiring and unit tests. - Frontend: add metrics types, API client + tests, Pinia store, and a new Metrics dashboard route/view wired into the sidebar.
- UI: provide basic charts/tables and filter controls (board + relative date range).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/taskdeck-web/src/views/MetricsView.vue | New metrics dashboard UI (filters, summary cards, charts, tables, states). |
| frontend/taskdeck-web/src/types/metrics.ts | TS interfaces for metrics query/response DTOs. |
| frontend/taskdeck-web/src/api/metricsApi.ts | API client for fetching board metrics with query params. |
| frontend/taskdeck-web/src/tests/api/metricsApi.spec.ts | Tests for metrics API URL/query construction + encoding. |
| frontend/taskdeck-web/src/store/metricsStore.ts | Pinia store for loading/error/data state and demo-mode behavior. |
| frontend/taskdeck-web/src/router/index.ts | Adds lazy-loaded /workspace/metrics route. |
| frontend/taskdeck-web/src/components/shell/ShellSidebar.vue | Adds “Metrics” nav item to sidebar. |
| backend/src/Taskdeck.Application/DTOs/BoardMetricsDtos.cs | Defines query/response records for board metrics. |
| backend/src/Taskdeck.Application/Services/IBoardMetricsService.cs | Service contract for board metrics computation. |
| backend/src/Taskdeck.Application/Services/BoardMetricsService.cs | Implements metrics computation and authorization enforcement. |
| backend/src/Taskdeck.Api/Controllers/MetricsController.cs | Authenticated API endpoint for board metrics with defaults. |
| backend/src/Taskdeck.Api/Extensions/ApplicationServiceRegistration.cs | Registers IBoardMetricsService with authorization dependency. |
| backend/tests/Taskdeck.Application.Tests/Services/BoardMetricsServiceTests.cs | Unit tests for validation and basic metric computations. |
Comments suppressed due to low confidence (1)
backend/src/Taskdeck.Application/Services/BoardMetricsService.cs:187
- Blocked duration is estimated using
DateTimeOffset.UtcNow - c.UpdatedAt, butUpdatedAtchanges for many unrelated card updates after a block is set (including label edits and moves), which will reset the duration and make the metric incorrect. Consider persisting a dedicatedBlockedAt/UnblockedAttimestamp on the card, or computing durations from audit log history (e.g., parseAuditAction.Updatedchange summaries for "Blocked:" / "Unblocked"); also captureUtcNowonce per request to keep results consistent within a response.
return new BlockedCardSummary(
c.Id,
c.Title,
c.BlockReason,
Math.Round(blockedDuration, 2));
})
.OrderByDescending(b => b.BlockedDurationDays)
.ToList();
return (blockedCards.Count, blockedCards);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var doneColumn = columns.OrderByDescending(c => c.Position).FirstOrDefault(); | ||
|
|
||
| var throughput = ComputeThroughput(cards, doneColumn, query.From, query.To); | ||
| var (avgCycleTime, cycleTimeEntries) = ComputeCycleTime(cards, doneColumn, query.From, query.To); | ||
| var wipSnapshots = ComputeWip(columns, cards); | ||
| var totalWip = wipSnapshots.Sum(w => w.CardCount); | ||
| var (blockedCount, blockedCards) = ComputeBlocked(cards); | ||
|
|
||
| return Result.Success(new BoardMetricsResponse( | ||
| query.BoardId, | ||
| query.From, | ||
| query.To, | ||
| throughput, | ||
| avgCycleTime, | ||
| cycleTimeEntries, | ||
| wipSnapshots, | ||
| totalWip, |
There was a problem hiding this comment.
auditLogs are queried but never used, and throughput/cycle time are computed from the card’s current ColumnId and UpdatedAt. Since UpdatedAt is touched for many non-completion events (title/description edits, blocking/unblocking, reordering), this will miscount completions and distort cycle time. Consider deriving “entered done” timestamps from AuditLog entries (e.g., AuditAction.Moved with changes containing target_column=...) and using those timestamps for throughput bucketing and cycle-time calculations; if audit logs aren’t needed, remove the query to avoid wasted work.
| public void ComputeThroughput_ShouldReturnEmpty_WhenNoDoneColumn() | ||
| { | ||
| var result = BoardMetricsService.ComputeThroughput( | ||
| new List<Card>(), | ||
| null, | ||
| DateTimeOffset.UtcNow.AddDays(-7), | ||
| DateTimeOffset.UtcNow); | ||
|
|
||
| result.Should().BeEmpty(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ComputeCycleTime_ShouldReturnZero_WhenNoDoneColumn() | ||
| { | ||
| var (avg, entries) = BoardMetricsService.ComputeCycleTime( | ||
| new List<Card>(), | ||
| null, | ||
| DateTimeOffset.UtcNow.AddDays(-7), | ||
| DateTimeOffset.UtcNow); | ||
|
|
||
| avg.Should().Be(0); | ||
| entries.Should().BeEmpty(); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void ComputeWip_ShouldCountCardsPerColumn() |
There was a problem hiding this comment.
The tests only cover “no done column” cases for throughput/cycle time and don’t assert any positive scenarios where cards enter the done column. Adding tests that simulate card completion via AuditAction.Moved (and verify that subsequent non-move updates don’t change throughput/cycle time) would help prevent regressions and would catch issues caused by relying on Card.UpdatedAt/current ColumnId.
| background: var(--td-color-ember); | ||
| border-radius: var(--td-radius-sm); | ||
| transition: width 0.3s ease; | ||
| min-width: 4px; |
There was a problem hiding this comment.
The WIP bar fill has min-width: 4px, so columns with cardCount === 0 will still render a visible bar (misrepresenting zero WIP). Consider conditionally removing the min-width for zero values (or using min-width: 0 and relying on the count label) so the chart accurately reflects empty columns.
| min-width: 4px; | |
| min-width: 0; |
|
|
||
| <!-- Empty state --> | ||
| <div v-else-if="!hasData && canFetch" class="td-metrics__state"> | ||
| <p>No metrics data available. Select a board to get started.</p> |
There was a problem hiding this comment.
This empty-state copy is shown when a board is already selected (canFetch is true) but metrics is still null. “Select a board to get started” is misleading in that situation; consider changing the message to something like “No metrics available for this board/date range” (or a demo-mode-specific message if applicable).
| <p>No metrics data available. Select a board to get started.</p> | |
| <p>No metrics available for this board and date range.</p> |
Coverage for branches was 70.26%, below the 71% global threshold. Add unit tests for metricsStore (normal + demo mode) and MetricsView (14 tests covering all template branches: loading, error, empty, dashboard, empty charts, blocked alert, null block reason, watcher triggers). Branch coverage now at 72%.
Adversarial Review -- PR #667 (ANL-01: Board Metrics)BLOCKING1. Cycle time uses
Fix: Use the audit log ( 2. "Done" column detection assumes rightmost column is always "done" Fix: Either let users mark a column as "done" (domain change), or use a heuristic that checks the column name (fragile but better), or document this assumption prominently and add a parameter for the done column ID. SHOULD-FIX3. Interface placed in 4. Constructor allows 5. All cards loaded into memory regardless of date range 6. 7. Frontend: metricsStore swallows demo mode silently 8. Blocked duration estimate is unreliable MINOR9. Throughput groups by 10. 11. No 12. Frontend types use Summary: The two BLOCKING issues (#1 and #2) mean the metric values displayed to users will frequently be incorrect. The core computation logic needs to use a more reliable signal for "card completed" than |
…ristic, auth hardening - Use audit log (AuditAction.Moved) to determine actual card completion timestamps for throughput and cycle time instead of unreliable UpdatedAt. Falls back to UpdatedAt when no audit data exists. - Resolve done column by matching well-known names (done, complete, finished, closed, shipped, released) case-insensitively before falling back to rightmost column by position. - Remove nullable IAuthorizationService constructor to prevent accidental auth bypass; authorization is now always enforced. - Change From >= To validation to From > To to allow same-day queries. - Show explicit "Metrics are not available in demo mode" message in frontend instead of silently returning null data. - Use UtcDateTime.Date for throughput date bucketing to avoid timezone issues. - Add tests: done column resolution, audit-based throughput/cycle time, ParseTargetColumnId, auth service error propagation, same-day query.
Bump TESTING_GUIDE.md to 2026-04-02 and update verified totals and re-verification dates (backend 2053, frontend 1496, combined 3549+). Add detailed coverage notes for recent PRs: LLM Tool-Calling (PR #669), MCP Server (PR #664), GDPR Data Portability/Account Deletion (PR #666), Board Metrics (PR #667), and GitHub OAuth frontend tests (PR #668), including tracking issues, test targets, and brief manual validation steps. Record addition of a developer-portal CI artifact step (PR #658) and expand planned quality expectations to include the new services and API/store coverage for OAuth and metrics.
Summary
Implements GitHub Issue #77 — ANL-01: Board metrics dashboard with throughput, cycle time, WIP, and blocked card trends.
Backend
GET /api/metrics/boards/{boardId}?from=&to=&labelId=— authenticated, authorization-checked endpoint with sensible defaults (last 30 days)Frontend
/workspace/metricswith lazy loadingTest plan
dotnet build backend/Taskdeck.sln -c Release— 0 errorsdotnet test backend/Taskdeck.sln -c Release -m:1— all pass (including 12 new metrics tests)npm run typecheck— cleannpm run build— successfulnpx vitest --run— 1495 tests pass (4 new metricsApi tests)Closes #77